Skip to content

Support list expansion (in @ids) via SqlMapper.PackListParameters - #197

Merged
mgravell merged 5 commits into
mainfrom
list-expansion
Aug 20, 2026
Merged

Support list expansion (in @ids) via SqlMapper.PackListParameters#197
mgravell merged 5 commits into
mainfrom
list-expansion

Conversation

@mgravell

Copy link
Copy Markdown
Member

An expandable (enumerable) member previously bound as a single raw parameter, which fails at execution. The generated AddParameters now delegates to SqlMapper.PackListParameters, which owns the whole in-list contract: the SQL rewrite (empty-list and optimize-hint forms included), per-item parameters, DbString items, padding and string_split settings, and provider array support.

Design note for review: PackListParameters is public but [Obsolete("intended for internal usage")] — I suppress CS0618 with a per-line pragma in the generated code. Calling the existing API means this works against every shipped Dapper, with no feature-detection diagnostic needed. The alternative is a fresh non-obsolete wrapper in Dapper, probe-gated with DAP052 like the DynamicParameters.AddParameters(IDbCommand) overload (DapperLib/Dapper#2225); happy to go that way instead if we'd rather not lean on an obsolete member from generated code.

Guard rails, all parse-side so those call-sites stay on vanilla Dapper rather than misbehave:

  • command caching is disabled for a factory with an expandable member (parameter shape varies per call), and CanPrepare is cleared;
  • multi-exec over elements with an expandable member is skipped — batch reuse updates parameters in-place, which cannot re-expand a list whose size changed between items;
  • an expandable member alongside an output/return parameter is skipped — PostProcess reads those back by index, and expansion shifts every index after it.

Also fixes a cosmetic CS0168: the shared p local in AddParameters is now only declared when some member still uses it.

New ListExpansion fixture covers the three working shapes and both skips; the TsqlTips golden moves off the raw bind, which was the broken behaviour. Verified against the Dapper test-suite harness: all the in-list ParameterTests (PassInIntArray, Issue220, Issue192, the string_split group, TestListOfAnsiStrings) now pass on both SqlClient providers.

…n PackListParameters

An expandable (enumerable) member previously bound as a single raw parameter, which
fails at execution; now the generated AddParameters delegates to the public (obsolete,
"library usage only") SqlMapper.PackListParameters, which owns the whole in-list
contract: the SQL rewrite (including the empty-list and optimize-hint forms), per-item
parameters, DbString items, padding and string_split settings, and provider array
support. Calling the existing API means this works against every shipped Dapper, so no
feature-detection diagnostic is needed; the alternative - a fresh non-obsolete wrapper
in Dapper, probe-gated like the DynamicParameters overload - is a fair follow-up if we
would rather not lean on an [Obsolete] member from generated code.

Guard rails, all parse-side so the call-sites stay on vanilla Dapper rather than
misbehave:
- command caching is disabled for a factory with an expandable member (the parameter
  shape varies per call), and CanPrepare is cleared;
- multi-exec over elements with an expandable member is skipped (batch reuse updates
  parameters in-place, which cannot re-expand a list whose size changed);
- an expandable member alongside an output/return parameter is skipped (PostProcess
  reads those back by index, and expansion shifts every index after it).

The shared "p" local in AddParameters is now emitted only when some member still
needs it, since a factory whose members all expand otherwise declares it unused
(CS0168 in the consumer's build).

New ListExpansion fixture covers the three working shapes and both skips; the
TsqlTips golden moves off the raw bind, which was the broken behaviour.
mgravell added a commit that referenced this pull request Aug 18, 2026
mgravell added a commit that referenced this pull request Aug 19, 2026
* Start the Dapper/Dapper.AOT parity accounting under notes/

The goal on record: enable Dapper.AOT in the Dapper test suite, announce
types via attributes, and have it swallow everything - AOT-clean.

- parity.md: the feature table, with impact/complexity per gap (several
  'gaps' score zero because the concept doesn't exist under AOT, e.g.
  the ref-emit plan cache)
- tokens.md: @ids expansion, {=literal}, ?foo? pseudo-positional, param
  filtering
- type-vs-generic.md: the announced-types design space for Type-based APIs
- test-suite-audit.md: the Dapper tests as acceptance corpus, sequenced

* GetTypeDeserializer is valid API, not cache plumbing

With announced types it's the same dispatch map (boxed materializer), and
its generic strengthening already exists as GetRowParser<T>. The real hole
is the write side: CreateParamInfoGenerator has no generic counterpart -
recorded the GetParameterBinder<T> proposal, and the question of blessing
CommandFactory<T>/RowFactory<T> as the supported surface. ReadChar and
friends are plain AOT-safe statics, nothing to do.

* Scope the accounting to the public API and observable behavior

PublicAPI.Shipped.txt is the checklist; the contract is what reaches the
provider and what comes back, never Dapper's internals. Cuts both ways:
the dynamic row needs behavioral fidelity only (the type is internal),
while the public infrastructure statics ARE in scope because extenders
call them.

* Record the decision: internals-asserting tests get adjusted, not maintained

* New work item: warn (new DAP id) on use of the has-no-meaning APIs

Plan-cache surface, CommandFlags.NoCache, possibly ConnectionStringComparer:
supported-and-meaningless under AOT, which is a different statement to
DAP001's unsupported-but-meaningful. Warning, not error - the code runs.

* Measurement caveat: build-time DAP counts are an upper bound

Some failure modes are silent until executed - handled means intercepted,
not correct. Only the DB-backed test run catches silent divergence.

* First harness baseline: 'handled 396 of 396' alongside 96 compile errors

Two root-cause generator bugs (array-of-anonymous parameter emits the
display string and wrecks the parse; inaccessible row types are emitted
rather than refused), plus two scorecard honesty problems (the denominator
excludes unattempted APIs; handled does not mean compiles) and a zero-
analyzer-diagnostics anomaly to re-check once the compile is clean.

* Generator audit: the capture model snapshots Roslyn nodes; fix first

Both generators' cached SourceState hold IMethodSymbol/ITypeSymbol/
Location (MemberMap even holds an IOperation), and the pipeline combines
the raw CompilationProvider into the source output - so it behaves as a
full-recompute generator with a memory leak. Recorded as a sequencing
gate ahead of the gap-closing features, with the fix shape that worked
for protobuf-net (plain equatable model, span-based locations, separate
diagnostics branch, shape-enforcing test).

* Record the agreed plan: gap table, then generator model, then features

The line that resolves the phase-1/2 tension: nothing that adds
parse-time state lands before the model rework completes; refusals and
scorecard fixes are allowed ahead of it, which is what lets phase 1 see.

* Round 2 numbers, and log the modern-interceptor-syntax work item

* Round 3: the suite compiles with AOT enabled (4 fix PRs + 2 severity downgrades)

* Scoreboard: all three TFM legs compile; local SQL Server available

* Work item: [UnsafeAccessor] may lift the accessibility refusals (net8+)

* Round 4: the honest scorecard says 53%, not 100%

* Harvest the skip breakdown; flag the DAP016 corpus-shape decision

* Round 5: first behavioral run - 84 failures, every one compiled clean

* Note that aot-harness is deliberately local-only

* Phase 2 log: approach and increments

* Phase 2 log: increment 1 done

* Phase 2 log: 3a done

* Phase 2 log: 3b done

* Phase 2 log: 3c-i done; two cached symbols remain

* Phase 2 log: result-side plan done; one symbol left

* Phase 2 log: cached model fully plain; only increment 4 remains

* Phase 2 log: complete - PRs #187 + #188

* Phase 2 log: caching tests landed

* Phase 2 log: readonly-field quirk fixed (#190)

* Round 6: DAP051 + restructure takes interception to 68.1%

* Round 6b: 612/760 behavioral; failures track interception growth honestly

* DynamicParameters design: delegate to the bag; needs one small Dapper API

* Round 7: DynamicParameters at 73.5%; First-pipeline drain divergence found

* Round 7b: 612/762; every failure class maps to a planned feature

* Interceptor-syntax migration: soft-target requirement recorded

* Tokens: runtime-SQL design - per-factory memoized role scan

* Record the feature-detection rule (DAP052) in the design note

* Round 8: CommandBehavior parity fix (PR #196)

* Round 8b: 616/762, suite loop 17s

* Round 9: list expansion lands (PR #197), 638/762

* Round 10: custom parameters + the two bugs they uncovered, 658/793

* Round 11: dynamic-record fidelity, 672/793

* Sync with main; point parity rows at their open PRs

The table lands on main via #186; from here each feature PR flips its own
cells, so the table and the merge history cannot drift apart. Rows with an
open PR say so, and the flip to a settled status is that PR's job.
…198)

* Support ICustomQueryParameter members: the value adds itself

A member implementing SqlMapper.ICustomQueryParameter (a TVP from
AsTableValuedParameter being the common case, on a DataTable or an
IEnumerable<SqlDataRecord>) previously bound as a single raw parameter;
now the generated AddParameters calls value.AddParameter(command, name),
which is the whole vanilla contract. A null reference-typed member throws
with vanilla's exact message; a struct member gets no null test. The
command cannot be prepared (no declared DbType).

The list-expansion guards generalise to cover this: expandable and custom
members are both 'self-binding' - they contribute an unknowable number of
parameters - so the same parse-side rules apply (no command caching; skip
multi-exec; skip alongside output/return parameters, which PostProcess
reads back by index). The shared p local pre-scan learns the new member
kind too.

New CustomParameters fixture covers interface-typed, class and struct
members, a custom member alongside a plain one, and both skips.

* Tick the parity cells this lands
@mgravell
mgravell merged commit b098b3c into main Aug 20, 2026
2 checks passed
mgravell added a commit that referenced this pull request Aug 26, 2026
* DAP000: separate refused-with-diagnostics from skipped-silently

The scorecard had two buckets, "unsupported API" and "skipped due to
diagnostics", and the second was not true of everything in it. A call-site can
be dropped with nothing said at all, and those were being counted as though a
diagnostic had explained them.

Measured on the Dapper suite, the old line read "82 unsupported API, 110
skipped due to diagnostics". It now reads "82 unsupported API, 75 refused with
diagnostics, 35 skipped silently" - so a third of the skips had no explanation
attached, and the instrument we have been steering by said otherwise.

Three sources, all now visible rather than inferred:

- CommandDefinition overloads. The analyzer only inspects call-sites with a
  string `sql` parameter (or one marked [Sql]); these carry the SQL inside the
  struct, so it never validates them and never reports. The generator sees them
  - it filters by method name - so they are counted, just not explained.
  IsVisibleToAnalyzer mirrors that entry condition so the generator can tell
  which of its skips anything will have reported.
- the self-binding guards from #197 and #198 (multi-exec over an expandable
  member; expandable alongside an output parameter). Documented in the fixture
  comments, invisible to consumers.
- GetRowParser's concreteType overload.

Adds CommandDefinitionOverloads as a pinned fixture, so the count moves if that
changes. Every golden .txt shifts with the message, and DAP004 asserts DAP000's
arguments explicitly, so it gains the new one.

Not fixed here: driving the silent count to zero. That needs a diagnostic at
each of those sites, which is a separate change - this one makes the number
visible so it can be driven down and kept there.

* Derive the gap table: classify Dapper's whole public surface, and pin it (#212)

The parity table's statuses were remembered rather than checked, and the two I
probed by hand this week were both understated. This replaces the guessing for
the question that can be answered mechanically - "what does Dapper.AOT do with
this overload?" - and leaves parity.md the judgement it is actually good for:
impact, complexity, and decisions like the runtime-registration non-goal.

The dispatch decision turns out to depend only on the method symbol, never on
the call-site, so IsDapperMethod gains a symbol overload and the test classifies
all 110 public SqlMapper extension methods without synthesising a single call.
No database, no harness, runs in a second.

The five dispositions, and what they found:

  candidate                      40  generation is attempted
  unsupported API (diagnosed)    16  refused, DAP001 names it
  unsupported API (undiagnosed)  13  refused, nothing says so
  skipped silently               27  dropped mute - vanilla under JIT, runtime
                                     failure under AOT, no build-time signal
  not inspected                   9  outside the name filter: AsList, Parse,
                                     GetTypeName and friends, which is correct -
                                     they need no interception

So 40 of 110 overloads give a consumer nothing to act on, almost all of them
CommandDefinition-shaped. That is the finding; fixing it is separate work.

The report is checked in and compared on every run, so an overload added
upstream fails the test rather than passing unnoticed, and a row moving into
'skipped silently' shows up in a diff.

Bounded deliberately: this says what happens to an *overload*, not to every call
of one - a supported overload can still be refused at a call-site for reasons of
its own. And it says nothing about behaviour; only the Dapper suite does that.
@mgravell
mgravell deleted the list-expansion branch September 11, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant